-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code to add WSLProfiles. #1050
Code to add WSLProfiles. #1050
Conversation
…moved the try block to the calling function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK with this.
I see pros and cons to shelling out to the wsl.exe to get the list of distributions.
On the pro side, you don't have to be responsible for knowing where the distribution information is stored, how it is parsed, or any of that. Especially as it has the potential to change going forward.
On the con side, it feels very hacky to be running another utility and scraping the output it gives because the format of that output now becomes sort of an API or a protocol itself.
Overall, I am fine with this for now because it gets us a step closer to what we want and I'm not sure the other solutions aren't without pitfalls as well. This method is very easy to understand and easy to fix. And if someone has a major qualm, it's not hard to replace the body of the _AppendWslProfiles
method with something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm largely okay with this, though I kinda want an answer on the icon before I'll sign-off
DWORD exitCode; | ||
if ((GetExitCodeProcess(pi.hProcess, &exitCode) == false) || (exitCode != 0)) | ||
{ | ||
THROW_HR(E_INVALIDARG); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A non-zero exit code will indicate that there are no distribuitons. This is not an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you rather have this throw something so that it gets logged CATCHLOG()
? I'm thinking ERROR_NO_DATA
would be descriptive enough as to what's happening.
For the time being, I've changed it to return without error.
…ment variable. Removed incorrect error useage. Removed variable that was not required.
I think we're all pretty happy with this change, so I'm going to merge it. If anyone has any remaining qualms, lets file follow-up issues |
Summary of the Pull Request
This code generates a list of WSL Distributions to be added to the drop down list. It was tested manually with a setup holding a single WSL distribution.
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
While implementing the stream I was getting stuck in a loop inside of the
wfstream
functions when I ran past the end of the output. None of thewfstream
's health functions seemed to be working properly (::rdstate()
,::eof()
, etc) so I usedpeeknamedpipe
to get the length and stopped reading from the pipe before this became an issue. I'd like to find a more clean method